feat(tab): modular ionic migration and lazy-load specs#31115
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
package: core
thetaPC
changed the title
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me! Just highlighting an issue with a test that I think could be improved
|
|
||
| it('should not retry attach after a failed first attempt', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tab], | ||
| html: '<ion-tab tab="home" component="ion-content"></ion-tab>', | ||
| }); | ||
|
|
||
| const tabEl = page.body.querySelector('ion-tab') as any; | ||
| const attachViewToDom = jest.fn().mockRejectedValue(new Error('attach failed')); | ||
| tabEl.delegate = mockDelegate(attachViewToDom); | ||
|
|
||
| // First call rejects because the delegate throws. The loaded flag is already | ||
| // set to true before the attempt, so the failure is permanent. | ||
| await expect(tabEl.setActive()).rejects.toThrow('attach failed'); | ||
| await tabEl.setActive(); | ||
|
|
||
| expect(attachViewToDom).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
Is the "permanent failure" behavior actually what we want here, or just what the code currently does? The synchronous try/catch in prepareLazyLoaded can't catch the async rejection from attachComponent, and this.loaded = true is set before the await, so any first-attempt failure locks the tab out for good. A passing test on this makes a future fix (resetting loaded on rejection, or moving the catch around an await) look like a regression. Worth either filing a follow-up and referencing it here, or renaming the test so it reads "documents current behavior" rather than "asserts a contract"?
There was a problem hiding this comment.
I added TODOs to go back and fix this on a separate ticket: 575d355
2 participants
Issue number: resolves internal
What is the current behavior?
ion-tabretains a@virtualProp themeJSDoc annotation that is no longer appropriate after the Modular Ionic migration. Also no spec tests exist for the component's lazy-load behavior.What is the new behavior?
@virtualProp themefromion-tabattachComponentis only called once across multiplesetActive()calls, and documents the permanent-failure behavior when the delegate rejects on the first attemptDoes this introduce a breaking change?
Other information
Since I was already here, I added spec tests that would be beneficial.
Previews